Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add counter to cache_id #1797

Merged

Conversation

stillwaiting
Copy link
Contributor

Current (in master) version of generate_cache_id is collision-prone: for complex factories with multiple
uploaders we have quite a few flaky specs because of that. Having a counter, this and similar situations, like mentioned here, should be resolved completely.

@@ -178,7 +189,9 @@ def workfile_path(for_file=original_filename)
alias_method :full_original_filename, :original_filename

def cache_id=(cache_id)
raise CarrierWave::InvalidParameter, "invalid cache id" unless cache_id =~ /\A[\d]+\-[\d]+\-[\d]{4}\z/
# Earlier cache_id contained only 3 pieces, the "counter" part was introduced later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to talk about later. Change to "Earlier version used 3 part cache_id."


class CacheCounter
@@counter = 0
def self.increment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please insert a linebreak before this method definition.

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
@thomasfedb
Copy link
Contributor

@bensie can I get your thoughts on this?

@thomasfedb
Copy link
Contributor

CI is failing on JRuby and Rails master, both of which are expected.

@stillwaiting
Copy link
Contributor Author

Seems they are failing for quite a while, e.g. here I can see the same picture: https://travis-ci.org/carrierwaveuploader/carrierwave/builds/96401284

@thomasfedb
Copy link
Contributor

@stillwaiting yes, that was what I mean, that those cases also fail on master and the issue is not introduced by your changes.

@thomasfedb
Copy link
Contributor

GTTM @bensie

@mehlah
Copy link
Member

mehlah commented Dec 30, 2015

@thomasfedb @stillwaiting The build should be 💚 now thanks to #1807.
I reviewed the changes, and it looks good to me 👍

thomasfedb added a commit that referenced this pull request Dec 30, 2015
Add a counter to generated cache_id values to avoid collisions
@thomasfedb thomasfedb merged commit bcbedf0 into carrierwaveuploader:master Dec 30, 2015
@mtsmfm
Copy link

mtsmfm commented Jan 24, 2016

@thomasfedb How about backporting?
Our tests will fail if collision occurs 😢
Many carrierwave users may tackle this random failure.

@thomasfedb
Copy link
Contributor

@mtsmfm I'm very happy to consider back porting this to the 0.10-stable branch, although we need to get the CI build fixed first.

@mtsmfm
Copy link

mtsmfm commented Jan 25, 2016

OK, I'll investigate that 😉

@mehlah
Copy link
Member

mehlah commented Jan 27, 2016

@mtsmfm should be good with #1847 cc @thomasfedb.
We can start backporting some fixes for an intermediate release.

@mtsmfm
Copy link

mtsmfm commented Jan 27, 2016

@mehlah Thanks for letting me know 😄

mtsmfm pushed a commit to mtsmfm/carrierwave that referenced this pull request Feb 2, 2016
------------------

Add counter to cache_id

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
mtsmfm pushed a commit to mtsmfm/carrierwave that referenced this pull request Feb 3, 2016
------------------

Add counter to cache_id

Old version of generate_cache_id was collision-prone: for complex factories with multiple uploaders we had quite a few flaky specs because of that. With a counter, this (and similar situations like carrierwaveuploader#1232) should be resolved completely.
thomasfedb added a commit that referenced this pull request Feb 3, 2016
Backport cache counter_id (#1797) to 0.11-stable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants